-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lodash: Refactor away from _.unionBy()
#43735
Conversation
Size Change: +50 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
4a3459b
to
65d49f5
Compare
() => | ||
[ | ||
...( settingsBlockPatterns || [] ), | ||
...( restBlockPatterns || [] ), | ||
].filter( | ||
( x, index, arr ) => | ||
index === arr.findIndex( ( y ) => x.name === y.name ) | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is simple from an implementation perspective, but it is O(n^2)
...
Given that we have 4 uses of this in the same package, perhaps it would be worth having a utility method instead, that checked for uniqueness on insert? It should be sub-quadratic (O(n log n)
, I believe) if it keeps the keys in an auxiliary Set
and checks that for each item in the arrays upon insertion.
What do you think, @tyxla?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't come up with a helper method because this is actually in 2 separate packages - @wordpress/edit-site
and @wordpress/editor
. So if we had a helper method, we'd have to either move it somewhere and export it - something I'm consciously avoiding with this migration - or, repeat it, which is never cool, especially if the method is more than a couple of lines. Seems like a check on insertion would be a bit more complex than what we came up with here. A simplified version of it would be something like:
const result = [ ... settingsBlockPatterns ];
restBlockPatterns.forEach( ( value ) => {
if( ! result.some( e => e.name === value.name ) ) {
result.push( value );
}
} );
That being said, I'm open to suggestions for improving the implementation - do you have anything specific in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed the fact that this is two different packages, you're right 🤔
Your new suggestion is still O(n^2)
, however.
The simplest functional approach with sub-quadratic complexity I can come up with is:
restBlockPatterns.reduce(
( acc, pattern ) => {
const { names, patterns } = acc;
if ( ! names.has( pattern.name ) ) {
names.add( pattern.name );
patterns.push( pattern );
}
return acc;
},
{
names: new Set( settingsBlockPatterns.map( p => p.name ) ),
patterns: [ ...settingsBlockPatterns ],
}
).patterns;
This will be O(n log n)
or O(n)
, depending on whether Set.prototype.has
is O(log n)
or O(1)
.
That said, it is far more involved than your original approach, so I'll leave it to your judgment. Realistically, this is probably not going to be a hot path, so code simplicity probably wins over algorithmic complexity concerns here 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you expressed my thoughts here: we can make this faster, but it's not worth the reduction of readability, so I prefer to go with the simplicity. Not to mention that the unionBy()
original function appears to have quite the complexity (it's pretty complex), but I haven't spent time going through it and calculating it.
65d49f5
to
b1161cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you, @tyxla 👍
() => | ||
[ | ||
...( settingsBlockPatterns || [] ), | ||
...( restBlockPatterns || [] ), | ||
].filter( | ||
( x, index, arr ) => | ||
index === arr.findIndex( ( y ) => x.name === y.name ) | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed the fact that this is two different packages, you're right 🤔
Your new suggestion is still O(n^2)
, however.
The simplest functional approach with sub-quadratic complexity I can come up with is:
restBlockPatterns.reduce(
( acc, pattern ) => {
const { names, patterns } = acc;
if ( ! names.has( pattern.name ) ) {
names.add( pattern.name );
patterns.push( pattern );
}
return acc;
},
{
names: new Set( settingsBlockPatterns.map( p => p.name ) ),
patterns: [ ...settingsBlockPatterns ],
}
).patterns;
This will be O(n log n)
or O(n)
, depending on whether Set.prototype.has
is O(log n)
or O(1)
.
That said, it is far more involved than your original approach, so I'll leave it to your judgment. Realistically, this is probably not going to be a hot path, so code simplicity probably wins over algorithmic complexity concerns here 👍
What?
This PR removes the
_.unionBy()
usage completely and deprecates the function.Why?
Lodash is known to unnecessarily inflate the bundle size of packages, and in most cases, it can be replaced with native language functionality. See these for more information and rationale:
@wordpress/api-fetch
package haslodash
as a dependency #39495How?
_.unionBy()
is easily replacible with a native filter implementation. We're also fixing a few tests - namely, improving the response for the block pattern category endpoint to return an array instead of an object, which corresponds more properly to the original endpoint schema.Testing Instructions